#2145 — fix(core): make timing masks safe for source loaders

Verdict: LGTM 🟢

Test-only PR, 16 lines. The new test proves idempotency of compileTimingAttrs when source HTML contains JavaScript replacement-pattern specials ($&, $$, $\`, $') inside <style> and <script> blocks. Double-compiling preserves inert content verbatim and doesn't leak HFMASK placeholders.

Covers a real edge case — String.prototype.replace treats those tokens as back-references; the mask/unmask round-trip must be transparent to them.

Review by Miga

Final direct pass at current head 38d62f7. Verified the requested stack-specific behavior and no unresolved current review threads remain. Current CI lanes are green; any displayed regression failure is from a superseded run, while the latest shard set passes. Approved on code merits; retain stack order.

Reviewed at 38d62f77.

Test-only pin for the existing maskInertRegions mechanism in timingCompiler.ts:91. The new re-compilation idempotency + dollar-sequence fixture at timingCompiler.test.ts:171 is a real guard, not enshrined defense — I traced the internals:

• Every internal .replace(...) on masked HTML uses a function as the 2nd arg (lines 93, 100, 176, 183, 191, 226), so String.prototype.replace's $&/$$/$`/$' substitution rules don't fire on the masked content. The only string-arg replacements (line 78 injectAttr, lines 300/305 injectDurations) receive programmatic numeric/token values, not user content, so dollar substitution can't land untrusted characters there either.
• The mask itself uses �HFMASK<n>� NUL-delimited tokens — the comment at timingCompiler.ts:87 says the raw 0x00 bytes previously "shipped once and broke every render (issue #2139)", so the � escape form is load-bearing to survive Bun's transpiler. The new test asserts no HFMASK residue after two passes, which catches any future accidental reintroduction.
• Assertion set is precise: (1) <style> content survives verbatim, (2) <script> content survives verbatim, (3) real <video class="hero"> still gets data-end="2" injected, (4) no HFMASK residue. All four exercise actual observable behavior — this isn't a .toHaveBeenCalled style over-mock.

One question on scope: the PR body says "Restrict timing rewrites to real element tags and add adversarial source-loader fixtures." The diff only does the second half — the "restrict to real element tags" mechanism (i.e., maskInertRegions) already lives at timingCompiler.ts:91-102 on the base. If the restriction landed in an earlier PR of the 31-stack, the body copy is fine as a stack-level narrative; if not, the "restrict" verb is claiming work that isn't in this diff. Either is easy to clarify.

LGTM from my side.

— Review by Rames D Jusso

Verdict: 🟡 LGTM-with-title-nit at 38d62f7

Endorses Miga + Rames-D — the double-compile round-trip + String.prototype.replace dollar-token fixtures at timingCompiler.test.ts:172-186 are a real lock-in of an existing invariant (maskInertRegions already uses callback-form replace at timingCompiler.ts:93,100,176,183,191,226, which sidesteps $-substitution). Adding one calibration note the prior reviewers didn't surface.

Findings

F1. Conventional-commit type drift — nit, non-blocking. Diff is one file, timingCompiler.test.ts +16/-0. Zero production LOC. The production fix (maskInertRegions + HFMASK\d+ NUL delimiters) already exists on the base branch 585aa9f and was actually shipped in #2140. fix(core): conventional-commit type implies a user-facing bug fix visible in changelogs; accurate type here is test(core):. Semantic-PR-title check passes on format alone, so this isn't a merge blocker — but a title fix via REST PATCH /repos/{owner}/{repo}/pulls/{number} keeps release-note tooling honest. Miguel's PR-envelope lens (feedback_hf_review_scope_pr_envelope.md in memory) treats this drift as request-changes on principle; I'm calibrating to nit given the fix is one API call away.

F2. PR body overstates scope — nit. Body's "How" says: "Restrict timing rewrites to real element tags and add adversarial source-loader fixtures." The first clause implies production changes; only the second matches the diff. Reads cleaner as: "Add adversarial round-trip test for existing inert-region masking (#1940, #2140) — no production change."

F3. Comment coverage gap in the fixture set — nit. INERT_REGION_RE at timingCompiler.ts:87 masks <!--...--> HTML comments in addition to <script> and <style>. The new test only exercises dollar-token fixtures inside script+style. Existing it("ignores media tags mentioned inside comments (issue #1938)") covers comment-with-media at base line 143 but not comment-with-dollar-tokens. Not blocking — callback-form replace is content-agnostic, so the invariant transitively holds — but a <!-- $&$$ --> fixture would lock the comment path in symmetrically.

Adversarial pass

Initial pull was to raise F1 as request-changes under strict fix(core): reading. Dropped to nit because (a) format check will pass, (b) fix: for a regression test locking a real prior fix is defensible-adjacent, (c) the fix is one REST PATCH away with no rebase risk. Considered dropping F2 as noise; kept because scope-drift signals in the body compound with F1 in the changelog. F3 initially felt load-bearing; adversarial pass demoted it because callback-form replace is content-agnostic and the transitive proof is trivial.

R1 by Via

Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.

Reviewed the final head against its actual parent 45dfcd58. This boundary is exactly the 16-line regression test; it does not smuggle production changes from the wider stack.

packages/core/src/compiler/timingCompiler.test.ts:172-185 now pins the dangerous replacement-string sequences ($&, $$, $\``, $'`) inside both script and style regions, recompiles the output, verifies real media timing still compiles, and asserts no mask residue. I also verified the parent already restores masks with callback-form replacement, so the test is guarding the intended existing fix rather than passing over string-replacement corruption.

Verification: all 25 timing-compiler tests passed locally; all 48 final-head GitHub check runs completed without failure. The previously raised test-only title/body nits remain metadata observations, not code defects.

Verdict: COMMENT (post-merge: Ready)
Reasoning: The own-diff boundary is test-only and the adversarial fixture directly proves the source-loader corruption class stays closed.

— Deepwork